Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add island labels (#399, #646, #703) #737

Merged
merged 36 commits into from
Apr 28, 2016
Merged

add island labels (#399, #646, #703) #737

merged 36 commits into from
Apr 28, 2016

Conversation

nvkelso
Copy link
Member

@nvkelso nvkelso commented Apr 21, 2016

Connects with #399 to add island, islet & other labels. Also refactors how the earth layer is constructed.

  • [query] add earth queary JINJA
  • [query] hook up schema prefunction for new earth layer
  • [query] hook up create sql functions and sql.jinja to new earth layer
  • [filter] remove continent place labels from places layer
  • [filter] init earth YAML filter, add continent place label to earth layer
  • [filter] adjust island, islet zoom ranges
  • [apply] hook up data/apply functions, set new fields for new earth layer
  • [tests] add full set of island label tests
  • [migration] add custom migration for new earth layer features
  • [docs] add new kinds to earth layer docs

@nvkelso nvkelso added this to the v0.10.0 milestone Apr 21, 2016
@nvkelso
Copy link
Member Author

nvkelso commented Apr 21, 2016

@zerebubuth This is still a WIP, but it's pretty close to done. Please have a look around, particularly at migration and query parts.

I've verified the zooms are working well in the DB for the test cases, but still need to fully run the test via python.

@nvkelso
Copy link
Member Author

nvkelso commented Apr 21, 2016

Also connects to #703 to move the continent label placement from places layer to earth layer.

@nvkelso
Copy link
Member Author

nvkelso commented Apr 21, 2016

Also connects to #646 to move the last layer, earth, to the new YAML setup.

zerebubuth
zerebubuth reviewed Apr 21, 2016
View reviewed changes
queries/earth.jinja2
max(gid) AS __id__
max(gid) AS __id__,
NULL AS label_placement,
NULL AS boundary
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we prefer to do the addition of extra parameters (label_placement, boundary) via YAML now, to avoid having to do NULL AS ... in the other queries.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll revise.

(This is loosely based off the water.jinja2 file, probably similar cleanup over there, but I'd rather not mess with that in this PR.)

@nvkelso nvkelso force-pushed the nvkelso/399-add-island-labels branch from d8cba1a to 5c11337 Compare April 25, 2016 01:27
nvkelso added 19 commits April 25, 2016 13:18
@nvkelso
[filter] remove continent place labels from places layer
77e5983
@nvkelso
[filter] init earth YAML filter, add continent place label to earth l…
3949c71 
…ayer
@nvkelso
[query] add earth queary JINJA
1306fb5
@nvkelso
[migration] add custom migration for new earth layer features, includ…
e287f5d 
…ing islands, islets & etc
@nvkelso
[query] hook up schema prefunction for new earth layer
841fdaf
@nvkelso
[query] hook up create sql functions and sql.jinja to new earth layer
ed2f448
@nvkelso
[apply] hook up data/apply functions, set new fields for new earth layer
933c809
@nvkelso
[tests] add full set of island label tests
478d994
@nvkelso
[filter] adjust island, islet zoom ranges
a32f242
@nvkelso
improve legibility
a3bf4af
@nvkelso
improve legibility
b3efd26
@nvkelso
correct typo
a9f760c
@nvkelso
rename way_area to test, since it is a fakeout
34e47eb
@nvkelso
[docs] add new earth kinds (label placements)
0046ec4
@nvkelso
use else, drop unused label_placement & etc in selects
18b4ef1
@nvkelso
remove junk & add comments
17944dc
@nvkelso
calculate label placement for islands as a transform, drop the src
ffccbbe
@nvkelso
add island names to tests, reformatting, fix z17 tests to z16
3bd9499
@nvkelso
update migrations
0d3e119
@nvkelso nvkelso force-pushed the nvkelso/399-add-island-labels branch from c698f2e to 6a01957 Compare April 25, 2016 12:22
@nvkelso
Copy link
Member Author

nvkelso commented Apr 25, 2016

I've got it all working (!?) except for these handful of tests which fail:

Similar test do pass, though, and the features exist in the DB and seem to be marked with the expected zoom values.

The synthetic_columns: - test bit should also be investigated & resolved.

Because I'll be traveling the rest of the week, please pick up this PR and see it to completion. Thanks!

@nvkelso nvkelso changed the title add island labels (#399) add island labels (#399, #646, #703) Apr 25, 2016
@zerebubuth
Copy link
Member

Tests pass for me. I think all the issues above should be fixed now. @rmarianski could you review this, please?

@rmarianski
Copy link
Member

👍

@zerebubuth zerebubuth merged commit 7718f17 into master Apr 28, 2016
@zerebubuth zerebubuth deleted the nvkelso/399-add-island-labels branch April 28, 2016 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@zerebubuth zerebubuth

Labels
None yet
Projects
None yet
Milestone
v0.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@nvkelso @zerebubuth @rmarianski